Skip to content

fix: event listener leak in helper.js - #7975

Open
Jetshree wants to merge 1 commit into
sugarlabs:masterfrom
Jetshree:fix-el-stacking
Open

fix: event listener leak in helper.js#7975
Jetshree wants to merge 1 commit into
sugarlabs:masterfrom
Jetshree:fix-el-stacking

Conversation

@Jetshree

@Jetshree Jetshree commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

This pr fixes an event listener leak in planet/js/helper.js.

Each time hideOnClickOutside() was called for the same anchor element it added a new document click listener without removing the previous one, due to this duplicate listeners stacked unnecessarily.

I added a module scoped WeakMap to track the active listener for each anchor element.
Before adding a new listener, the existing one is removed which ensure only one outside click listener exists per anchor at a time.

PR Category

  • Bug Fix — Fixes a bug or incorrect behavior
  • Feature — Adds new functionality
  • Performance — Improves performance (load time, memory, rendering, etc.)
  • Tests — Adds or updates test coverage
  • Documentation — Updates to docs, comments, or README
  • Chore / Refactor — Maintenance, cleanup, or refactoring with no behavior change
  • CI/CD — Changes to CI/CD workflows and automation

Testing Performed

Before: repeatedly opening and closing the share boxes kept adding new document click listeners
image

After: listener count consistently stayed at the baseline
image

Additional Notes for Reviewers

  • The baseline of three document click listeners on initial page load is expected and comes from existing application initialization (materialize css and core tab handlers)
  • The WeakMap garbage collection ensures memory safety when DOM elements are removed.

@Jetshree
Jetshree requested a review from zealot-zew as a code owner August 5, 2026 22:39
@github-actions github-actions Bot added bug fix Fixes a bug or incorrect behavior tests Adds or updates test coverage size/M Medium: 50-249 lines changed area/tests Changes to test files labels Aug 5, 2026
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.06%. Comparing base (5462731) to head (5833ac0).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7975      +/-   ##
==========================================
+ Coverage   62.05%   62.06%   +0.01%     
==========================================
  Files         187      187              
  Lines       58377    58394      +17     
==========================================
+ Hits        36227    36244      +17     
  Misses      22150    22150              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/tests Changes to test files bug fix Fixes a bug or incorrect behavior size/M Medium: 50-249 lines changed tests Adds or updates test coverage

Projects

Development

Successfully merging this pull request may close these issues.

1 participant